Skip to content

Revert "Extensible pagination token implementation"#2123

Closed
eric-maynard wants to merge 1 commit intomainfrom
revert-1938-pagination-alt-flex
Closed

Revert "Extensible pagination token implementation"#2123
eric-maynard wants to merge 1 commit intomainfrom
revert-1938-pagination-alt-flex

Conversation

@eric-maynard
Copy link
Contributor

@eric-maynard eric-maynard commented Jul 16, 2025

Reverts #1938

After pulling in this change, I see errors when trying to create a catalog on main:

LD4RTJ0HY9:polaris emaynard$ ./polaris \
>     catalogs \
>     create quickstart_catalog \
>     --storage-type file \
>     --default-base-location 'file:///tmp/quickstart'
Exception when communicating with the Polaris server. (500)
Reason: Internal Server Error
HTTP response headers: HTTPHeaderDict({'content-type': 'application/json; charset=utf-8', 'content-length': '72'})
HTTP response body: {"details":"Error id afec5ba7-078c-4917-8d4d-2feaf219fe13-1","stack":""}

The server logs point to changes from this PR:

2025-07-16 10:01:26,449 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] [,POLARIS] [,,,] (executor-thread-1) HTTP Request to /api/management/v1/catalogs failed, error id: afec5ba7-078c-4917-8d4d-2feaf219fe13-1: java.util.ServiceConfigurationError: com.fasterxml.jackson.databind.Module: Provider com.fasterxml.jackson.module.scala.DefaultScalaModule could not be instantiated
	at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:586)
	at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:813)
	at java.base/java.util.ServiceLoader$ProviderImpl.get(ServiceLoader.java:729)
	at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
	at com.fasterxml.jackson.databind.ObjectMapper.findModules(ObjectMapper.java:1161)
	at com.fasterxml.jackson.databind.ObjectMapper.findModules(ObjectMapper.java:1145)
	at com.fasterxml.jackson.databind.ObjectMapper.findAndRegisterModules(ObjectMapper.java:1195)
	at org.apache.polaris.core.persistence.pagination.PageTokenUtil.<clinit>(PageTokenUtil.java:46)
	at org.apache.polaris.core.persistence.pagination.PageToken.readEverything(PageToken.java:70)
	at org.apache.polaris.service.admin.PolarisAdminService.listCatalogsUnsafe(PolarisAdminService.java:964)

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jul 16, 2025
@eric-maynard
Copy link
Contributor Author

eric-maynard commented Jul 16, 2025

Confirmed that catalog creation succeeds again after this revert.

@dimas-b dimas-b requested a review from snazy July 16, 2025 18:32
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 16, 2025
@dimas-b
Copy link
Contributor

dimas-b commented Jul 16, 2025 •
edited
Loading

Good catch! Fixing this (unexpected) breakage makes sense to me.

@eric-maynard : shall we redo the pagination PR with the above error in mind?

@dimas-b
Copy link
Contributor

dimas-b commented Jul 16, 2025 •
edited
Loading

The alternative to reverting, I guess, is to fix-forward, but I am personally not available to do this fix today 😅

@snazy
Copy link
Member

snazy commented Jul 16, 2025

+1 to fix-forward

@snazy
Copy link
Member

snazy commented Jul 16, 2025

@eric-maynard do you have a reproducer as a test?

@eric-maynard
Copy link
Contributor Author

eric-maynard commented Jul 16, 2025 •
edited
Loading

The CLI command in the description reproduces the error from the description when Polaris is running (I used ./gradlew run) -- do you see the same error?

We cannot break main and try to fix-forward the next day.

@eric-maynard
Copy link
Contributor Author

eric-maynard commented Jul 16, 2025

I'll work on a regtest, though -- I'm not sure why this would have passed CI if the error is real. We can hold this until then if it doesn't repro

@adam-christian-software
Copy link
Contributor

adam-christian-software commented Jul 16, 2025 •
edited
Loading

Hey, @eric-maynard. Thanks for finding this! I agree with you and I think we should revert (rather than fix-forward).

Here's what I'd recommend:

  1. Revert the PR
  2. It would be REALLY cool if we could have a test, so we could have blocked this pre-merge. Either, you could write that PR or we could file an item for it.
  3. Robert fixes up the pagination PR (I think it's late his time so he's probably off right now), so we don't regress this.
  4. Stepping back, it seems like we have some implicit norms about when a revert is appropriate versus when we should fix forward given the comments. I think it'd be interesting to chat about in our next community sync, so we could figure out what our community norms are going to be. I'll send an email to the Dev ML.

What do you think?

@eric-maynard
Copy link
Contributor Author

eric-maynard commented Jul 16, 2025

+1 @adam-christian-software, we can hold this until I have a test -- I left a comment on the original PR, but I think the issue only occurs when you also have another Scala module on the classpath (like when using the Spark plugin). I'll close this and re-open a revert or fix-forward when there's a test.

@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jul 16, 2025
@snazy snazy deleted the revert-1938-pagination-alt-flex branch July 17, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dimas-b dimas-b dimas-b approved these changes

@snazy snazy Awaiting requested review from snazy

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eric-maynard @dimas-b @snazy @adam-christian-software